Skip to content

Add span-derived primary tags (CSS v1.3.0)#11402

Open
dougqh wants to merge 336 commits into
masterfrom
dougqh/metrics-arbitrary-tags
Open

Add span-derived primary tags (CSS v1.3.0)#11402
dougqh wants to merge 336 commits into
masterfrom
dougqh/metrics-arbitrary-tags

Conversation

@dougqh

@dougqh dougqh commented May 18, 2026

Copy link
Copy Markdown
Contributor

What Does This Do

Implements span-derived primary tags (Client-Side Stats v1.3.0): users configure DD_TRACE_STATS_ADDITIONAL_TAGS to extract matching span tag values as additional aggregation dimensions on ClientGroupedStats.AdditionalMetricTags.

Motivation

From the RFC...

Users may want to aggregate APM statistics by custom dimensions beyond the standard primary tag (typically env). For example, a user might want stats grouped by region, tenant_id, or other business-relevant span tags. This enables more granular visibility into service performance across different segments.

Configuration

This feature is experimental and must be explicitly opted in.

Environment Variable System Property Default Description
DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED dd.trace.experimental.features.enabled (none) Required to enable this feature. Set to DD_TRACE_STATS_ADDITIONAL_TAGS (comma-separate to enable multiple experimental features).
DD_TRACE_STATS_ADDITIONAL_TAGS dd.trace.stats.additional.tags (none) Comma-separated span tag keys to use as additional aggregation dimensions. Max 4 keys; excess dropped at startup with a warn log.
DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT dd.trace.stats.additional.tags.cardinality.limit 100 Per-key distinct-value cap per reporting cycle. Values beyond the cap are reported as tracer_blocked_value.

Minimal configuration to enable:

DD_TRACE_EXPERIMENTAL_FEATURES_ENABLED=DD_TRACE_STATS_ADDITIONAL_TAGS
DD_TRACE_STATS_ADDITIONAL_TAGS=region,tenant_id

Additional Notes

Design

Wire format: new AdditionalMetricTags field on ClientGroupedStats, emitted as repeated string of "<key>:<value>" entries (mirrors PeerTags). Schema-ordered (alphabetical by key); null slots skipped; field omitted when empty, so unconfigured deployments pay zero payload overhead.

Cardinality protection — three caps, matching the .NET implementation:

  • MAX_ADDITIONAL_TAG_KEYS = 4 — configured-key count cap; excess keys dropped at startup with a warn log.
  • Per-value length cap (200 chars) — values exceeding this are replaced with tracer_blocked_value.
  • DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT (default 100) — per-key distinct-value cap per reporting cycle; further new values collapse to the "<key>:tracer_blocked_value" sentinel.

Threading: all canonicalization (UTF8BytesString interning, cardinality limiting) runs on the aggregator thread. The producer path captures raw String values into a String[] parallel to the schema — no sync on the hot path.

Tests

Migrated SerializingMetricWriterTest from Spock/Groovy to JUnit 5 Java and folded the additional-tags coverage into its shared ValidatingSink — one msgpack-decode harness instead of two.

Benchmark

AdditionalTagsMetricsBenchmark.publish (JMH, Java 17, 8 threads, @Fork(1)):

limitsEnabled throughput
false 22.3M ± 0.7M ops/s
true 6.9M ± 1.4M ops/s

The true arm is slower because it records more — over-cardinality values collapse into the tracer_blocked_value sentinel — not because limiting itself is expensive.

🤖 Generated with Claude Code

@dougqh dougqh added type: feature Enhancements and improvements comp: metrics Metrics tag: ai generated Largely based on code generated by an AI or LLM labels May 18, 2026
@dougqh
dougqh force-pushed the dougqh/metrics-memory-efficiency branch from 46c04bd to 823a5d4 Compare May 18, 2026 19:28
@dougqh
dougqh force-pushed the dougqh/metrics-arbitrary-tags branch from c552e73 to 42947dd Compare May 18, 2026 19:30
@datadog-official

This comment has been minimized.

dougqh and others added 24 commits May 20, 2026 14:03
The iterator tests need a populated Hashtable.Entry[] to drive
Support.bucketIterator / mutatingBucketIterator. Relaxing D1.buckets
from private to package-private lets the same-package tests read it
directly, removing the reflection helper.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the TagMap pattern: pairs the existing forEach(Consumer) with a
forEach(T context, BiConsumer<T, TEntry>) overload so callers can hand
side-band state to a non-capturing lambda and avoid the
fresh-Consumer-per-call allocation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Factors the unchecked (TEntry) cast out of D1.forEach / D2.forEach (and
the BiConsumer variants) into Support.forEach(buckets, ...). The cast
now lives in one place, mirroring how Entry.next() handles it, and the
D1/D2 methods become one-liners. Downstream higher-arity tables built
on Support gain the same helper.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds Support.bucket(buckets, keyHash) which returns the bucket head
already cast to the caller's concrete entry type. D1.get and D2.get
now drop the raw-Entry intermediate variable and walk the chain via
Entry.next() directly. The unchecked cast lives in one place,
consistent with Entry.next() and Support.forEach.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Holdover from when both lived in a shared HashtableBenchmark; redundant
now that each lives in its own class.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bleIterator

Three consumer-facing helpers that callers building higher-arity tables on
top of Hashtable.Support kept open-coding:

- MAX_RATIO_NUMERATOR / _DENOMINATOR: the 4/3 multiplier for sizing a
  bucket array from a target working-set under a 75% load factor.
- insertHeadEntry(buckets, bucketIndex, entry): the (setNext + array-store)
  pair for splicing a new entry at the head of a bucket chain.
- MutatingTableIterator + Support.mutatingTableIterator(buckets): walks
  every entry in the table (not filtered by hash) with remove() support,
  for sweeps like eviction and expunge that aren't keyed to a specific
  hash. Sibling of MutatingBucketIterator.

Tests cover the table-wide iterator at head-of-bucket and mid-chain
removal, empty buckets between live entries, exhaustion, and
remove-without-next.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… create()

Replace Support.MAX_RATIO_NUMERATOR / _DENOMINATOR with a single float
MAX_RATIO constant, and add a Support.create(int, float) overload that
takes a scale factor. Callers now write Support.create(n, MAX_RATIO)
instead of stitching together the int arithmetic at the call site.

The scaled size is truncated (not ceiled) before going through sizeFor.
sizeFor already rounds up to the next power of two, so truncation just
absorbs float fuzz that would otherwise push a result like 12 * 4/3 =
16.0000005f past 16 and double the bucket array size for no reason.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five small cleanups from a design re-review pass:

1. Support javadoc: drop the stale "methods are package-private" sentence;
   most of them were made public in earlier commits for higher-arity
   callers. Also drop the "nested BucketIterator" framing (iterators are
   peers of Support inside Hashtable, not nested inside Support).
2. MAX_RATIO javadoc: drop the Math.ceil recommendation; create(int, float)
   deliberately truncates and is the canonical pathway.
3. Document the null-hash treatment on D1.Entry.hash and D2.Entry.hash so
   the behavior difference is explicit: D1 uses Long.MIN_VALUE as a
   sentinel that's collision-free against any int-valued hashCode(); D2
   has no such sentinel and relies on matches() to resolve null/null vs
   hash-0 collisions.
4. Rename Support.MAX_CAPACITY -> MAX_BUCKETS and sizeFor's parameter to
   requestedSize. The cap is on the bucket-array length, not entry count;
   the new name reflects that. Error messages updated to match.
5. Drop the `abstract` modifier on Hashtable in favor of `final` with a
   private constructor. Nothing actually subclasses Hashtable -- the
   abstract was a namespace device that read as "intended for extension."

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add Support.insertHeadEntry(buckets, long keyHash, entry) overload that
  derives the bucket index itself. Callers that already have a hash but
  not the index (the common case) now avoid the redundant bucketIndex(...)
  hop.
- D1.insert, D1.insertOrReplace, D2.insert, D2.insertOrReplace: use the
  new overload, drop the (thisBuckets local, bucketIndex compute,
  setNext, store) sequence at each call site.
- D2.buckets: drop the `private` modifier to match D1.buckets. Both are
  package-private so iterator tests in the same package can drive
  Support.bucketIterator against the table's bucket array. Added a short
  comment on both fields documenting the rationale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three follow-ups from the design review:

- Make Hashtable.Entry.next private. All same-package readers
  (BucketIterator) already had a next() accessor; the leftover direct
  field reads now route through it. Closes the "mixed encapsulation"
  gap where some readers used the accessor and same-package ones
  reached for the field.
- BucketIterator and MutatingBucketIterator now document that chain-walk
  work happens in next() (and the constructor for the first match);
  hasNext() is an O(1) field read.
- Add D1.getOrCreate(K, Function) and D2.getOrCreate(K1, K2, BiFunction).
  Both reuse the lookup hash for the insert on miss, avoiding the
  double-hash that "get; if null then insert" callers would otherwise
  pay.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses PR #11409 review comments:

- #3267164119 / #3267165525: wrap every single-line if/break body in
  braces (7 sites across BucketIterator, MutatingBucketIterator, and the
  full-table Iterator).

- #3275947761 / #3275948108 (sarahchen6): null out the removed/replaced
  entry's next pointer after splicing it out of the chain in
  MutatingBucketIterator.remove / .replace. Applied the same fix to the
  full-table Iterator.remove for consistency.

  Rationale: detaching prevents accidental traversal through a removed
  entry via a stale reference and lets the GC reclaim a chain tail that
  the removed entry was the last referrer to.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sistency

Addresses PR #11409 review comment #3276167001. The method parallels the
primitive hash(boolean) / hash(int) / hash(long) / ... family, so naming
it hash(Object) -- with null collapsing to Long.MIN_VALUE as a sentinel
distinct from any real hashCode -- matches the rest of the public surface.

Test call sites that pass a literal null now disambiguate against
hash(int[]) / hash(Object[]) / hash(Iterable) via an (Object) cast.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses sarahchen6's review comment on ConflatingMetricsAggregator
extractPeerTagPairs: replaces the worst-case-allocation + trim-and-copy
flat-pairs layout with a parallel-array carrier.

- New PeerTagSchema: minimal carrier of String[] names. Two flavors -- a
  static INTERNAL singleton (one entry: base.service) for internal-kind
  spans, and per-discovery built schemas for client/producer/consumer
  spans. Deliberately no cardinality limiters or per-cycle state; that
  layers on top in a later PR.

- ConflatingMetricsAggregator: caches the peer-aggregation schema keyed
  on reference equality of features.peerTags() -- a single volatile read
  + a long compare on the steady-state producer hot path, no allocation.
  The producer now captures only a String[] of values parallel to the
  schema's names; the schema reference is carried on SpanSnapshot. The
  prior "build worst-case pairs then trim" code is gone.

- SpanSnapshot: replaces String[] peerTagPairs with PeerTagSchema +
  String[] peerTagValues. Producer drops the schema reference if no
  values fired so the consumer short-circuits on null.

- Aggregator.materializePeerTags: now reads name/value pairs at the same
  index from (schema.names, snapshot.peerTagValues). Counts hits once
  for exact-size allocation; preserves the singletonList fast path for
  the common one-entry case (e.g. internal-kind base.service).

Producer-side cost goes from "allocate String[2n] + walk + maybe trim"
to "single volatile read + walk + lazy String[n] only on first hit".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Aggregator.materializePeerTags: fold the firstHit-discovery nested if
  into a single guarded post-increment (amarziali, #3279243138). One
  body line: `if (values[i] != null && hitCount++ == 0) firstHit = i;`.

- Drop redundant isKind(SpanKindFilter) overrides in both
  TraceGenerator.groovy files (amarziali, #3279264553 / #3279382648).
  CoreSpan.java:84 already supplies a default implementation that reads
  the same span.kind tag.

- Bump TRACER_METRICS_MAX_PENDING default from 2048 -> 131072 to address
  the capacity regression amarziali flagged (#3279378375). Without
  producer-side conflation, the inbox now holds 1 SpanSnapshot per
  metrics-eligible span instead of 1 conflated Batch per ~64 spans;
  restoring effective capacity parity (~2048 * ~64 = 131072) prevents a
  ~64x rise in inbox-full drops at the same span rate. ~100 B per
  SpanSnapshot puts the worst-case heap floor at ~13 MB -- bounded.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses PR #11381 review (amarziali, #3279325340 -- "Are the existing
tests covering this case?").

New ConflatingMetricsAggregatorInboxFullTest constructs the aggregator
with a small inbox (queueSize=8), deliberately does NOT call start() so
the consumer thread never drains, then publishes enough spans to
overflow the inbox. Verifies that healthMetrics.onStatsInboxFull() is
called at least once -- the fast-path's `inbox.size() >= inbox.capacity()`
short-circuit triggers when the producer-side queue is at capacity.

Test is Java + JUnit 5 + Mockito per the project convention for new
tests; uses a CoreSpan Mockito mock rather than the SimpleSpan Groovy
fixture so we don't depend on Groovy-then-Java compile order from the
test source set.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…read

Addresses amarziali's review comment #3279340181 ("It would be more
efficient to trigger from the other side"). The producer-side reference
compare on every publish goes away; the aggregator thread reconciles
the cached schema against feature discovery once per reporting cycle.

- DDAgentFeaturesDiscovery: expose getLastTimeDiscovered() so callers
  can detect a discovery refresh without copying the peerTags Set.

- PeerTagSchema: add `long lastTimeDiscovered` (plain, aggregator-only)
  and `hasSameTagsAs(Set)`. of(Set, long) takes the timestamp; INTERNAL
  uses a -1L sentinel since it's never reconciled.

- ConflatingMetricsAggregator:
  * Drop the cachedPeerTagsSource volatile and the per-publish reference
    compare.
  * Producer fast path is now `cachedPeerTagSchema` volatile read +
    null-check; first publish takes the one-time synchronized bootstrap.
  * Add reconcilePeerTagSchema() that runs once per cycle on the
    aggregator thread: fast-path timestamp compare, slow-path set
    compare, bump-in-place when the set is unchanged.

- Aggregator: new `Runnable onReportCycle` constructor parameter, run at
  the start of report() (before the flush, so any test awaiting
  writer.finishBucket() observes the schema in its post-reconcile state
  and so the next publish sees the new schema without a handoff).

- Update "should create bucket for each set of peer tags" to drive two
  reporting cycles separated by a report() that triggers reconcile. The
  old test relied on per-publish reference detection, which the new
  design intentionally doesn't preserve -- the schema is now stable
  within a cycle.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@bric3 bric3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there might be a few things to fix regarding the RFC conformance.

Also, shouldn't the OltStatsMetricWriter handle additional_metric_tags as well ?

Comment on lines +80 to +87
for (int i = 0; i < namesArr.length; i++) {
handlersArr[i] =
new TagCardinalityHandler(
namesArr[i],
limit,
useBlockedSentinel,
MetricCardinalityLimits.ADDITIONAL_TAG_MAX_VALUE_LENGTH);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: The RFC says that DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT is the number of distinct stat entries with additional tags per flush bucket.

But then, it seems the limit is applied by tag rather than "flush bucket" ? Could that make lots of combination. Or did I misread the RFC ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was some leeway left in the RFC to allow for some language variation.

The approach that I've taken has 3-4 levels of gates on it...

  • inbound stats queue has a hard limit on snapshots
  • each property / tag is limited on unique values during the digest cycle (10s)
  • each property / tag has a length limit
  • cap on distinct entries in the metrics table

The general aim is that the tag's collapse to sentinel values that indicate where the limit was imposed.
Then those sentinel values get folded into an entry, so entries start to fold together.

Comment thread internal-api/src/main/java/datadog/trace/api/Config.java
@mhlidd

mhlidd commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Actually there might be a few things to fix regarding the RFC conformance.

Also, shouldn't the OltStatsMetricWriter handle additional_metric_tags as well ?

@bric3 I believe Doug said he would add that in a follow-up. OTLP trace metrics is lower priority since it isn't officially released yet.

@dougqh

dougqh commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Actually there might be a few things to fix regarding the RFC conformance.
Also, shouldn't the OltStatsMetricWriter handle additional_metric_tags as well ?

@bric3 I believe Doug said he would add that in a follow-up. OTLP trace metrics is lower priority since it isn't officially released yet.

Thanks for thorough review. I think some of those things weren't specified in the RFCs when I originally started working on this. I tried having AI check, but clearly, some things were still missed. I'll address them tomorrow.

dougqh and others added 4 commits July 23, 2026 16:40
…fixes

- Rename PropertyHandlers -> CoreHandlers and resetPropertyHandlers/
  resetHandlers -> resetCoreHandlers; the meaningful axis is core (always
  present) vs peer (remote config) vs additional (local config), and
  property-vs-tag is an implementation detail of the core set. Reorder the
  AggregateTable/Canonical constructors so coreHandlers precedes
  additionalTagsSchema.
- Add MetricCardinalityLimits.USE_BLOCKED_SENTINEL: a compile-time escape
  hatch (shipped true) to revert to the pre-capping behavior during the
  internal rollout; wire all handler construction sites to it. Replaces the
  bogus MetricCardinalityLimits.ENABLED javadoc link.
- Move the previous-peer-tag-schema flush into reconcilePeerTagSchema(),
  ahead of the swap; drop "straggler"/"lockstep" jargon.
- Rename Canonical.populate -> populateFrom.
- Assorted javadoc/comment fixes (thread-confinement note, getDuration,
  post-increment style).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per the Span-Derived Primary Tags V1 RFC, an invalid (<= 0)
DD_TRACE_STATS_*_CARDINALITY_LIMIT must fall back to the default rather
than fail. getTraceStatsCardinalityLimit now clamps non-positive values
to the caller-supplied default and logs at debug; previously such a value
flowed into the handler constructors and threw IllegalArgumentException,
breaking ClientStatsAggregator construction.

Also correct the MetricCardinalityLimits javadoc, which wrongly claimed
the additional-tag limit is not configurable -- it is resolved via
DD_TRACE_STATS_ADDITIONAL_TAGS_CARDINALITY_LIMIT, matching the RFC.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The approved Cardinality Limits RFC keys collapse telemetry by the
lowercased protobuf field name. Two core-handler tags didn't match:

- operation is carried on the wire as the field "name", so its health
  tag is now collapsed:name (the human-facing CardinalityLimitReporter
  still says "operation"). PropertyCardinalityHandler gains a decoupled
  statsDField for this case.
- peer tags now aggregate into a single collapsed:peer_tags health
  metric across all configured peer tags (mirroring AdditionalTagsSchema),
  instead of emitting a per-tag-name collapsed:<tag> metric. Per-name
  detail is preserved in the human-facing reporter.

TagCardinalityHandler.statsDTag() is now unused and removed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Span-Derived Primary Tags RFC specifies the additional_metric_tags
field is present whenever the feature is configured -- as an empty array
for spans that matched no configured key -- not gated on whether a given
entry carried any tags.

SerializingMetricWriter previously omitted the field whenever the entry's
packed array was empty, which conflated "feature off" with "feature on,
no match". It now takes a configured flag (threaded from
AdditionalTagsSchema.size() > 0) and emits the field, empty array
included, whenever configured; when the feature is off the field is
omitted entirely so non-users pay zero payload overhead.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dougqh
dougqh requested a review from bric3 July 24, 2026 12:46

@bric3 bric3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few quibbles left but overall that's ok for me.

Comment thread dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java Outdated
dougqh and others added 2 commits July 24, 2026 10:43
The httpMethod/httpEndpoint/grpcStatusCode/additionalTagValues fields are
@nullable but their constructor params weren't, disagreeing with the
already-annotated peerTagSchema/peerTagValues params. Annotation-only;
no behavior change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- AggregateEntry.errorLatencies: note it is not thread-safe (bric3 suggestion)
- PeerTagSchema.resetHandlers: adopt bric3's clearer javadoc wording

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dougqh
dougqh enabled auto-merge July 24, 2026 15:43
@dougqh
dougqh added this pull request to the merge queue Jul 24, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Jul 24, 2026

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-07-24 15:44:51 UTC ℹ️ Start processing command /merge


2026-07-24 15:44:57 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in master is approximately 1h (p90).


2026-07-24 16:04:44 UTCMergeQueue: This merge request was updated

This PR is rejected because it was updated

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 24, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Jul 24, 2026

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-07-24 17:35:35 UTC ℹ️ Start processing command /merge


2026-07-24 17:35:40 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in master is approximately 1h (p90).


2026-07-24 18:25:08 UTCMergeQueue: The build pipeline contains failing jobs for this merge request

Build pipeline has failing jobs for 4749305:

⚠️ Do NOT retry failed jobs directly (why?).

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • If your PR checks are green, try to rebase/merge. It might be because the CI run is a bit old.
  • Any question, go check the FAQ.
Details

Since those jobs are not marked as being allowed to fail, the pipeline will most likely fail.
Therefore, and to allow other builds to be processed, this merge request has been rejected and the pipeline got canceled.

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 24, 2026
@dougqh

dougqh commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Yes, I went over that part meticulously earlier, but it has been more than a month ago now. I had Claude jog my memory — just so we make sure this is correct...

The short answer

The stale-schema use is intentional and safe, and for the normal case the schema does not get "lost in nature" — the previousPeerTagSchema one-cycle retention exists precisely to catch this scenario. There is a residual hole, but it's a pathological-timing telemetry-accounting imprecision, not a data-loss or correctness bug.

Why the stale schema itself is fine

The producer does one volatile read of cachedPeerTagSchema and holds that reference for the whole trace (fast path). If the aggregator swaps in a new schema a nanosecond later, that swap does not retroactively reach the in-flight producer — it already has its snapshot of the schema. The SpanSnapshot captures the raw peer-tag values on the producer thread (capturePeerTagValues) and carries a reference to the schema it read. It gets aggregated correctly under that schema. So the metric — hit counts, latencies, the aggregate entry — is never at risk. Using yesterday's peer-tag set for one straggler span is a rounding error, not corruption.

What "could get lost" is only the collapse counter

The thing that's timing-sensitive is the cardinality-collapse bookkeeping. register() (which increments the old schema's blocked-value counters) runs on the aggregator thread when the snapshot is processed. So a snapshot that captured the outgoing schema S1 but is processed after the swap will bump S1's counters after S1 is already on its way out.

reconcilePeerTagSchema() handles exactly this:

  • On a real tag change it flushes S1's counters, then retains S1 as previousPeerTagSchema for one more cycle rather than discarding it.
  • The next reconcile reports and only then discards it — so a snapshot that captured S1 just before the swap and lands in the following cycle still gets its late block counts flushed before S1 disappears.

That's the mechanism the previousPeerTagSchema field doc describes, and it directly answers "does this get lost after the swap" — no, for a ≤1-cycle straggle.

The residual hole

The retention is exactly one cycle. So the genuine gap is: a producer paused between reading S1 and queuing its snapshot for two or more full reporting cycles (~20 s+ at the default 10 s cadence), and a discovery-driven tag change occurring in that window. Then:

  • Cycle N: producer reads S1; aggregator swaps S1→S2, keeps previousPeerTagSchema = S1.
  • Cycle N+1: reconcile reports+discards S1.
  • Cycle N+2: producer finally resumes, snapshot references S1, aggregator calls register() on the now-orphaned S1 → those blocked-value increments are reported to nobody.

What's lost even then: only the cardinality-collapse count for that one straggler span's blocked values — not the span, not its aggregate. And it requires (rare discovery tag change) × (a 20 s+ scheduler pause mid-publish). Realistically negligible.

If we ever wanted to close it

I don't think it's worth it, but the options are (a) retain previousPeerTagSchema as a small ring of the last K schemas instead of one, or (b) have the snapshot carry a generation stamp and drop late block-counter updates whose schema is already retired. Both add state to defend against a >20 s thread stall coinciding with a rare config change, which fails the cost/benefit test given CSS's actual failure modes were OOM and whole-key data loss, not counter drift.

@dougqh
dougqh added this pull request to the merge queue Jul 25, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Jul 25, 2026

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-07-25 01:53:55 UTC ℹ️ Start processing command /merge


2026-07-25 01:53:59 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in master is approximately 1h (p90).


2026-07-25 02:01:58 UTCMergeQueue: The build pipeline contains failing jobs for this merge request

Build pipeline has failing jobs for 5e7bcf5:

⚠️ Do NOT retry failed jobs directly (why?).

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • If your PR checks are green, try to rebase/merge. It might be because the CI run is a bit old.
  • Any question, go check the FAQ.
Details

Since those jobs are not marked as being allowed to fail, the pipeline will most likely fail.
Therefore, and to allow other builds to be processed, this merge request has been rejected and the pipeline got canceled.

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 25, 2026
@dougqh
dougqh enabled auto-merge July 25, 2026 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: metrics Metrics tag: ai generated Largely based on code generated by an AI or LLM type: feature Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants